Skip to content

[pigeon] started parsing multiple parameters#417

Closed
gaaclarke wants to merge 17 commits into
flutter:masterfrom
gaaclarke:pigeon-multiple-arity
Closed

[pigeon] started parsing multiple parameters#417
gaaclarke wants to merge 17 commits into
flutter:masterfrom
gaaclarke:pigeon-multiple-arity

Conversation

@gaaclarke

@gaaclarke gaaclarke commented Jul 24, 2021

Copy link
Copy Markdown
Member

This PR just parses multiple parameters and raises an error if we use them. It doesn't actually generate code for multiple parameters, that will happen in a later PR.

issue: flutter/flutter#86971
depends on: #416

Pre-launch Checklist

  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@google-cla google-cla Bot added the cla: yes label Jul 24, 2021
@gaaclarke gaaclarke requested a review from stuartmorgan-g July 27, 2021 18:35
@gaaclarke gaaclarke marked this pull request as ready for review July 27, 2021 19:00
@gaaclarke gaaclarke force-pushed the pigeon-multiple-arity branch from 0e313b8 to c54c43d Compare July 29, 2021 20:35
@stuartmorgan-g

stuartmorgan-g commented Jul 30, 2021

Copy link
Copy Markdown
Collaborator

Before I review this, could you update the PR description to explain what the expected scope of this PR is? You reference an issue, but based on the title and an initial look it doesn't look like this fixes that issue, just does some amount of ground-work. I'm not sure what exactly to expect the code to be doing as I review it.

(As general feedback: I find it much, much easier to review PRs—as well as to investigate repo history later when trying to understand code—if PRs have descriptions that clearly explain what the PR is expected to accomplish. Just the title is rarely enough context for anyone other than the PR author, and issue vary wildly from "not nearly enough context" to "so long that it's nearly impossible to map from the issue to the intended scope of a single PR".)

@gaaclarke

Copy link
Copy Markdown
Member Author

Before I review this, could you update the PR description to explain what the expected scope of this PR is?

Yep, SGTM. Done.

Comment thread packages/pigeon/lib/pigeon_lib.dart Outdated
Comment thread packages/pigeon/lib/pigeon_lib.dart Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this abusing Field to get a type object (i.e., the class I asked for in the previous PR)? If not, I don't understand why a) it's a Field, and b) it has no name. (If it is, obviously this just further reinforces my belief that that class should exist.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, in a later PR I actually start using the name parameter. It wasn't needed at this time to make existing tests pass.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm still missing something; how can a field not have a name? I don't understand what that would mean.

@gaaclarke gaaclarke Aug 2, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before this PR the AST never stored the name of the argument to the method (it would just generate them). Now we have the ability to store it, but no generator actually reads the value, so it's inconsequential what the value is. In the later PR we read from those values: #425

@stuartmorgan-g stuartmorgan-g Aug 3, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm still not following.

A Field is described as being a field of a class. That suggests to me that the return value is intended to be used directly as a class field in one of the generated parameter objects (if I'm wrong about that, then Field doesn't seem like the right type). What does it mean to have a field in a class without a name? I don't understand, conceptually, what that would mean in any of the languages Pigeon generates. So I do not understand what this return object is actually representing. I.e., if asked I could not, based on having read this implementation, write a coherent declaration comment that explains what—conceptually, not in terms of the class and values—this method returns.

Either this makes sense in some way I don't currently understand, in which case it needs a clear comment, or the return value is currently expressing something which can't exist, which seems very confusing and thus not something that should be done.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooookay, I think I understand. There are 2 issues here:

  1. I'm using a Field here since an Argument has the exact same data as a Field. I twisted the meaning of a pigeon.Field and it is clashing with your Schema for Field and the docstring for pigeon.Field
  2. The incompleteness of the AST. You can see the AST as a representation of the input source code, and if you look at it that way, any discrepancy in information is a flaw. Or you can look at the AST as the input to the generators, where any unused data is waste. Third, it has the role of being the input to the validator that gatekeepers the generators. It has to navigate those 3 responsibilities. It is complete today as much as it needs to be to implement the features we've delivered. There will probably always be gaps until we find the funding to fully flesh out more features.

Here's what we should do. I should introduce an Argument object here and use that instead. Sound good?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I twisted the meaning of a pigeon.Field and it is clashing with your Schema for Field and the docstring for pigeon.Field

I'm confusing why you are calling it "my" schema. It's identical to the one-sentence docstring—which you wrote, presumably—for Field. This isn't some esoteric meaning I've invented for it.

It has to navigate those 3 responsibilities.

You've missed an important responsibility, which is making sense to someone reading the code. If the representation isn't comprehensible to another reader—for instance, because it's abusing one concept to partially represent a different concept just to save a few lines of code—then that representation is problematic.

Here's what we should do. I should introduce an Argument object here and use that instead. Sound good?

That's fine, assuming there's a reason Field and Argument being different is actually useful. Otherwise just calling Field Variable instead, and changing its description, seems more straightforward.

@gaaclarke gaaclarke force-pushed the pigeon-multiple-arity branch from 1b6a429 to f6dd824 Compare August 3, 2021 20:03
@gaaclarke

Copy link
Copy Markdown
Member Author

subsumed by #428

@gaaclarke gaaclarke closed this Aug 3, 2021
austinstoker pushed a commit to austinstoker/packages that referenced this pull request Apr 29, 2022
	- and some related improvements
	- closes flutter#417
stuartmorgan-g pushed a commit that referenced this pull request Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants